Add checksums #541
Add checksums #541
Conversation
There was a problem hiding this comment.
Looking good! I suggest some changes to the modelling (which will affect how this is persisted too). I also thought about if we wanted to store multiple checksums for a version, say an md5 and sha256. Would a list of Checksums be better than the option?
One last thing, can you please try to rebase your feature branch rather than merge from master? As it stands now, it will cause multiple merge commits in our timeline on master.
| vendor: Option[Vendor] = None | ||
| vendor: Option[Vendor] = None, | ||
| hashAlgorithm: Option[HashAlgorithm] = None, | ||
| checksum: Option[String] = None |
There was a problem hiding this comment.
I'm not sure if that entirely makes sense. Can a checksum exist without an algorithm? These two together should probably have a case class of their own, and that should be an option.
Something like:
case class Checksum(algorithm: HashAlgorithm, value: String)
...
checksum: Option[Checksum]
There was a problem hiding this comment.
Agreed. But if this is slowing down the pull request, I'd be to contribute an edit.
There was a problem hiding this comment.
@stewSquared perhaps you aren't getting the context here, but you are looking at the database migration project. This will be the last thing to be implemented. It is also the least important of the lot.
We are driving this feature from the client side, and are currently focusing on the CLI. After that we will do the crucial work in the broker, followed by all the other auxillary services.
At the moment, everything works on the client, with the exception of one small bug that we've encountered with cached archives. The fix will be released soon. Checksums will be released into the wild when we think it's ready, not a moment sooner.
Store checksum info for downloadable artifacts (see #540)
This PR adds 2 new fields to the
versionsdocument to store the SHA algorithm and hash for each downloadable artifact. It also includes a migration step to update the checksums for Maven artifacts. If you guys are ok with this general direction I'll add checksums for the rest of the candidates and update the README.mdThis change is needed before submitting PRs to update the broker, cli and vendor-release repositories.